-
Notifications
You must be signed in to change notification settings - Fork 7
Fix limits and printing of subnormal_min #796
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
I was just opening a new bug for printing... if you have time can you check if this now works as expected: |
For: we get: Denorms were being treated as non-finite values. |
Nice, thank you for the fix. |
|
@libbooze I believe I hit all of your comments |
| static_assert(test_value(std::numeric_limits<decimal128>::denorm_min(), "1.0000000000000000000000000000000000000000e-6176")); | ||
| static_assert(test_value(std::numeric_limits<decimal128_fast>::denorm_min(), "1.0000000000000000000000000000000000000000e-6143")); | ||
|
|
||
| static_assert(test_value(std::numeric_limits<decimal32>::max() + std::numeric_limits<decimal32>::lowest(), "0.0000000000000000000000000000000000000000e+00")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: add same comment(// Lowest + max) as in other preprocessor branch to keep it consistent
yes, thank you. Left just one comment regarding comment. |
I'll add it. This needs a little more investigation because I think something has gone wrong with fpclassify now. The fuzzer seems like it's consistently hitting an assertion. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #796 +/- ##
=========================================
+ Coverage 98.9% 98.9% +0.1%
=========================================
Files 226 227 +1
Lines 16949 16967 +18
Branches 1807 1809 +2
=========================================
+ Hits 16749 16776 +27
+ Misses 200 191 -9
... and 2 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
|
Not sure if this is related to this issue, so feel free to not address this in this pull, but I noticed another issue with printing. Just mentioning it in case you might find it useful. on my machine prints: |
|
Can you open as a separate issue? I think this one has already exceeded its original scope since it wipes out a few issues. I'll have to compare print to regular io streams to see if they're identically wrong. The format support is the newest part of the library and got merged in like a week before review. |
Sure, but will wait till this is merged so I can test on develop. Easier to track/reproduce then. |
Closes #794